src: delete process.env values set to undefined #18158

targos · 2018-01-15T14:05:04Z

Before this commit, setting a property of process.env to undefined
would set the value to the string "undefined". This changes the
behavior to instead remove the value, working like the delete operator.

Refs: #15089

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

process

cjihrig · 2018-01-15T14:37:32Z

src/node.cc

@@ -2715,6 +2715,18 @@ static void EnvGetter(Local<Name> property,
 static void EnvSetter(Local<Name> property,
                      Local<Value> value,
                      const PropertyCallbackInfo<Value>& info) {
+  if (value->IsUndefined()) {
+#ifdef __POSIX__


Is it possible to use EnvDeleter() instead of duplicating the code?

targos · 2018-01-15T15:02:49Z

@cjihrig Not sure how. Two things to consider:

EnvDeleter ignores non-string properties whereas EnvGetter and EnvSetter convert them to strings.
EnvDeleter takes a const PropertyCallbackInfo<Boolean>& info parameter and I don't think I can construct one.

I pushed a change to move the code to another function, PTAL.

cjihrig

Code LGTM.

joyeecheung · 2018-01-15T16:31:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12545/

mhdawson

EDIT - Removed my approval as I don't feel strongly enough that this should land given the objections.

thefourtheye · 2018-01-16T11:11:06Z

doc/api/process.md

@@ -899,6 +904,10 @@ process.env.TEST = 1;
 delete process.env.TEST;
 console.log(process.env.TEST);
 // => undefined
+process.env.TEST = 1;
+process.env.TEST = undefined;
+console.log(process.env.TEST);


Instead of this, would it be better to show that the property TEST doesn't exist anymore in process.env, like in the test? As it is, it may not convey the idea clearly (setting undefined and then undefined is printed).

like this?

console.log('TEST' in process.env); // => false

Yes. That's better, right?

Before this commit, setting a property of process.env to undefined would set the value to the string "undefined". This changes the behavior to instead remove the value, working like the delete operator. Refs: nodejs#15089

targos · 2018-01-23T12:15:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12686/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1221/

TimothyGu

Even if this is semver-major, I still think this behavioral change is too significant to be done on process.env, especially when we previously documented to do the contrary. Plus, as #16961 demonstrated, we can never have a truly Correct environmental variable implementation with a string-based API. My opinion for this is, instead of adding exceptions to a previously internally consistent class, we should just leave process.env alone.

The current behavior is not without precedent either. The Web Storage API (better known as Local Storage and Session Storage) also cast undefined to string.

mcollina · 2018-01-29T04:13:40Z

I'm adding this to the next TSC agenda.

Fishrock123 · 2018-01-31T17:13:06Z

Setting undefined to undefined seem fine to me and is very consistent with how javascript generally works.

What about other non-string values? Shouldn't this throw or something?

> process.env.PWD = {}
{}
> process.env.PWD
'[object Object]'

What about null?

jasnell · 2018-01-31T19:08:53Z

I'm torn on this one because I really like the idea of deleting but setting to undefined is probably the right thing. I'm reluctantly -1 on this but hopeful that a really convincing argument can convince me to switch that :-)

BridgeAR · 2018-02-01T17:28:26Z

I agree with @TimothyGu and I am also -1 on this.

mhdawson · 2018-02-05T16:43:50Z

This was discussed at the last TSC meeting. We are going to invite Timothy to this week's meeting and then call for a vote.

rvagg · 2018-02-07T17:13:59Z

I like this, but without being able to attend to hear discussion I'll register an abstention for now unless it comes back for a vote.

mhdawson · 2018-02-07T22:25:38Z

Discussed in TSC meeting today. Still differing views, lets see if we can get Timothy at the next meeting before having a vote.

cjihrig · 2018-02-19T18:10:41Z

Can someone on the endorsing-this-PR side speak to that?

I'm pretty much in the same boat as @addaleax. I think it should have worked like this from the start. If this PR doesn't land, I wouldn't be too upset though.

joyeecheung · 2018-02-19T20:02:33Z

I don't feel strongly for this to land, although the comment in #18158 (comment) should be addressed

What about null?

It feels even stranger if we treat null and undefined differently.

Don't feel strongly that this should land at this point given the objections

gireeshpunathil · 2018-02-20T05:25:56Z

+1, better late than never. It is fair for a end user to expect

> process.env.LANG ? true : false
true
> process.env.LANG = undefined
undefined
> process.env.LANG ? true : false
false
>

rather than

> process.env.LANG ? true : false
true
> process.env.LANG = undefined
undefined
> process.env.LANG ? true : false
true
>

because that is how normal objects behave:

> var foo = {}
undefined
> foo.bar = 'hello'
'hello'
> foo.bar ? true: false
true
> foo.bar=undefined
undefined
> foo.bar ? true: false
false
>

mcollina

LGTM

TimothyGu · 2018-02-20T08:01:43Z

@gireeshpunathil Your argument is one with a slippery slope. What makes undefined special compared to null in your argument? Problem is, this restriction on undefined is entirely arbitrary. That’s no good way to design an API.

We should instead focus on creating a better API in general that allows us to tackle #16961, rather than retrofitting suboptimal designs onto an object that has wide-spread compatibility impact.

gireeshpunathil · 2018-02-20T08:32:31Z

What makes undefined special compared to null in your argument?

nothing, I don't discriminate null vs. undefined in this case - isn't it straightforward to fix null case as well, given we have the property setter?

Problem is, this restriction on undefined is entirely arbitrary. That’s no good way to design an API.

Existence of other, larger problem to be solved need not be a consideration for solving a problem for which fix is easy, right?

We should instead focus on creating a better API in general that allows us to tackle #16961, rather than retrofitting suboptimal designs onto an object that has wide-spread compatibility impact.

generally agree, but again, issue reported in #16961 need not hinder this from progressing, and both of them are not exclusive IMO.

gireeshpunathil · 2018-02-20T08:47:09Z

@TimothyGu - on a second thought, I don't believe this will have a wide-spread compatibility impact. Can you explain potential implications of this change in the field?

addaleax · 2018-02-20T08:50:45Z

What makes undefined special compared to null in your argument? Problem is, this restriction on undefined is entirely arbitrary.

@TimothyGu I’m in the “not too sad if this doesn’t happen” bucket, but I’m going to point out that many people (esp. language beginners) think of missing properties and properties with an undefined value as essentially the same – as in, setting a property to undefined is how you delete a property to them.

mcollina · 2018-02-20T09:45:45Z

@TimothyGu

We should instead focus on creating a better API in general that allows us to tackle #16961, rather than retrofitting suboptimal designs onto an object that has wide-spread compatibility impact.

Are you proposing a new API to improve the situation? I'm not sure that will help. A lot of modules rely on process.env, and I think adding a new API to access the same data is not feasible. We can make surgical changes to edge cases.

I think we are not providing a good experience to anyone relying on process.env behavior for undefined or null.

Side note, I think we might even throw if you are setting an object.

addaleax · 2018-02-20T10:46:50Z

Are you proposing a new API to improve the situation? I'm not sure that will help. A lot of modules rely on process.env, and I think adding a new API to access the same data is not feasible. We can make surgical changes to edge cases.

@mcollina A separate API is what is being suggested in #16961, although for a completely different reason.

Fishrock123 · 2018-02-21T17:02:41Z

This pull request has four Collaborators opposed to it

This is not true. I am not opposed to moving away from "undefined", but I would prefer having it set to undefined rather than deleting.

targos · 2018-02-21T17:09:19Z

This is not true. I am not opposed to moving away from "undefined", but I would prefer having it set to undefined rather than deleting.

I can look into this. Don't know if this is actually possible because AFAIK process.env is not really an object but a sort of C++ proxy acting like one.

Fishrock123 · 2018-02-21T17:10:09Z

Hmmm, well it's mostly just a preference, "undefined" seems clearly wrong to me.

addaleax · 2018-02-21T17:10:28Z

@Fishrock123 What would be stored in the actual environment in that case?

TimothyGu · 2018-02-22T00:16:11Z

@mcollina

I think we might even throw if you are setting an object.

Now that is something I can get behind, and I believe that is better than both this PR and the status quo. And in fact I think we can and should start with documentation-only deprecation of setting non-string to process.env's properties.

The compatibility impact is difficult to measure however. @ChALkeR ;)

@addaleax

I’m going to point out that many people (esp. language beginners) think of missing properties and properties with an undefined value as essentially the same – as in, setting a property to undefined is how you delete a property to them.

I see what you are saying. However, I'd like to contend that this PR doesn't help with the general issue of making process.env more friendly to language beginners. As a mentor to many Node.js and JavaScript workshops at my university, I have first-hand experience in the fact that an API where there are few or none exceptions is a much easier to teach and learn than one where certain values are considered "special." It's much easier to explain that "process.env converts everything to strings" than "process.env converts null, numbers, objects, and even functions to strings, but not undefined, because we consider undefined to be special."

Even worse, I understand that a lot of Node.js developers come from other JavaScript-based platforms, like the browser world, where localStorage, sessionStorage, and element.dataset have exactly the same behavior as process.env currently does with regards to stringification. Now, instead of explaining to them that "process.env works just like localStorage", we are deviating from behavior from both the browser world and the past to create one with arbitrary exceptions. And that's not okay.

For that reason, I believe throwing an error when setting non-string to process.env would be both more internally consistent and friendlier for people starting out.

@Fishrock123

I am not opposed to moving away from "undefined", but I would prefer having it set to undefined rather than deleting.

That would be even odder given the current behavior and implementation of process.env.

@gireeshpunathil

I don't believe this will have a wide-spread compatibility impact. Can you explain potential implications of this change in the field?

The point is that the impact will for sure be non-zero. And this, coupled with other disadvantages I pointed out before, forms the body of my objection.

mcollina · 2018-02-23T12:47:57Z

Now that is something I can get behind, and I believe that is better than both this PR and the status quo. And in fact I think we can and should start with documentation-only deprecation of setting non-string to process.env's properties.

I'm definitely 👍 on this approach. If you are setting process.env values that are not strings, it will should. I think we could even runtime-deprecate this in 10, but I'm ok to do it in 11 as well.

TimothyGu · 2018-02-25T21:51:57Z

I have proposed my approach to this issue in #18990.

mcollina · 2018-02-27T12:12:37Z

I think this can be closed in favor of #18990.
I've removed the tsc-agenda label.

targos · 2018-02-27T12:19:33Z

No objection to closing from me.

PR-URL: #18990 Refs: #15089 Refs: #18158 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

PR-URL: nodejs#18990 Refs: nodejs#15089 Refs: nodejs#18158 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 15, 2018

targos force-pushed the process-env-undefined branch from 40e3915 to 6c63403 Compare January 15, 2018 14:05

targos added semver-major PRs that contain breaking changes and should be released in the next major version. process Issues and PRs related to the process subsystem. labels Jan 15, 2018

targos requested a review from a team January 15, 2018 14:06

cjihrig reviewed Jan 15, 2018

View reviewed changes

joyeecheung approved these changes Jan 15, 2018

View reviewed changes

cjihrig approved these changes Jan 15, 2018

View reviewed changes

mhdawson previously approved these changes Jan 15, 2018

View reviewed changes

tniessen approved these changes Jan 15, 2018

View reviewed changes

thefourtheye reviewed Jan 16, 2018

View reviewed changes

thefourtheye approved these changes Jan 16, 2018

View reviewed changes

src: delete proccess.env values set to undefined

f88476a

Before this commit, setting a property of process.env to undefined would set the value to the string "undefined". This changes the behavior to instead remove the value, working like the delete operator. Refs: nodejs#15089

targos force-pushed the process-env-undefined branch from f49e58d to f88476a Compare January 23, 2018 12:14

TimothyGu suggested changes Jan 23, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 29, 2018

mhdawson mentioned this pull request Jan 29, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-01-31 nodejs/TSC#478

Closed

mhdawson mentioned this pull request Feb 5, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-02-07 nodejs/TSC#484

Closed

gireeshpunathil approved these changes Feb 20, 2018

View reviewed changes

mcollina approved these changes Feb 20, 2018

View reviewed changes

TimothyGu changed the title ~~src: delete proccess.env values set to undefined~~ src: delete process.env values set to undefined Feb 25, 2018

TimothyGu mentioned this pull request Feb 25, 2018

process: doc-only deprecate non-string env value #18990

Closed

4 tasks

mhdawson mentioned this pull request Feb 26, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-02-28 nodejs/TSC#496

Closed

mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 27, 2018

targos closed this Feb 27, 2018

targos deleted the process-env-undefined branch October 17, 2020 12:53

bnoordhuis mentioned this pull request Jul 21, 2023

Setting undefined to process.env.* sets a "undefined" string value #48875

Closed

src: delete process.env values set to undefined #18158

src: delete process.env values set to undefined #18158

Conversation

targos commented Jan 15, 2018

Checklist

Affected core subsystem(s)

cjihrig Jan 15, 2018

Choose a reason for hiding this comment

targos commented Jan 15, 2018

cjihrig left a comment

Choose a reason for hiding this comment

joyeecheung commented Jan 15, 2018

mhdawson left a comment • edited Loading

Choose a reason for hiding this comment

thefourtheye Jan 16, 2018

Choose a reason for hiding this comment

targos Jan 16, 2018

Choose a reason for hiding this comment

thefourtheye Jan 16, 2018

Choose a reason for hiding this comment

targos commented Jan 23, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

mcollina commented Jan 29, 2018

Fishrock123 commented Jan 31, 2018

jasnell commented Jan 31, 2018

BridgeAR commented Feb 1, 2018

mhdawson commented Feb 5, 2018

rvagg commented Feb 7, 2018

mhdawson commented Feb 7, 2018

cjihrig commented Feb 19, 2018

joyeecheung commented Feb 19, 2018

gireeshpunathil commented Feb 20, 2018

mcollina left a comment

Choose a reason for hiding this comment

TimothyGu commented Feb 20, 2018

gireeshpunathil commented Feb 20, 2018

gireeshpunathil commented Feb 20, 2018

addaleax commented Feb 20, 2018

mcollina commented Feb 20, 2018

addaleax commented Feb 20, 2018

Fishrock123 commented Feb 21, 2018

targos commented Feb 21, 2018

Fishrock123 commented Feb 21, 2018

addaleax commented Feb 21, 2018

TimothyGu commented Feb 22, 2018 • edited Loading

mcollina commented Feb 23, 2018

TimothyGu commented Feb 25, 2018

mcollina commented Feb 27, 2018

targos commented Feb 27, 2018

mhdawson left a comment •

edited

Loading

TimothyGu commented Feb 22, 2018 •

edited

Loading